Skip to content

fix(exports): verify every reported artifact - #270

Closed
nordic-style wants to merge 2 commits into
mixelpixx:mainfrom
nordic-style:codex/fix/verify-export-artifacts
Closed

fix(exports): verify every reported artifact#270
nordic-style wants to merge 2 commits into
mixelpixx:mainfrom
nordic-style:codex/fix/verify-export-artifacts

Conversation

@nordic-style

Copy link
Copy Markdown
Contributor

Summary

  • verify every schematic, PCB, Gerber, drill, BOM, and position artifact before reporting success
  • reject missing, empty, or non-regular output files even when kicad-cli exits successfully
  • build manufacturing responses exclusively from artifacts that passed verification
  • fix PCB PDF layer names and argument formatting so the requested document is actually produced

Closes #252

Approach

The export boundary now enforces a shared postcondition: every advertised artifact must exist, be a regular file, and contain data. Gerber and drill exports additionally require the expected output set, while project snapshots propagate either schematic or PCB export failure instead of returning phantom paths.

Manufacturing packages now expose complete; incomplete packages return an MCP error and omit upload instructions. The file list is assembled from verified paths rather than a raw directory scan, so stale or empty files cannot masquerade as current output.

This PR is intentionally stacked on #269. Its first commit is the export-option implementation from that PR; after #269 merges, the remaining diff is the focused artifact-verification commit cda33a3.

Compatibility and rollback

  • snapshot_project now fails when either requested PDF is missing or empty. This is an intentional correction of false-success behavior.
  • Requested drill output is mandatory for a complete manufacturing package.
  • Manufacturing responses add complete; partial packages use isError: true and no longer include upload instructions.
  • KiCad project sources remain read-only throughout export and validation.
  • After fix(manufacturing): apply Gerber and position options #269 merges, the behavior can be rolled back by reverting the single artifact-verification commit.

Validation

  • cargo test --workspace --locked --lib --tests: all new tests pass; the workspace reaches 555/558 konnect-core tests, with only the same three pre-existing local update_symbols_from_library_* failures caused by the installed KiCad Device:R library shadowing the test fixture
  • cargo test --workspace --locked --doc: pass
  • cargo clippy --workspace --locked --all-targets -- -D warnings: pass
  • cargo fmt --all -- --check: pass
  • real MCP manufacturing export against a temporary copy of the Voice Satellite six-layer board: complete: true, 14 verified non-empty artifacts, including all 11 requested Gerber plots plus PTH and NPTH drill files
  • real MCP project snapshot: verified non-empty schematic and PCB PDFs
  • fake-CLI regression tests confirm that a zero exit status without output fails closed

@mixelpixx

Copy link
Copy Markdown
Owner

#266 is merged and covers this branch's export_pdf rewrite (plus the SVG half this one lacked). The artifact-verification core here — issue #252, checking every reported export exists and is non-empty — is still wanted and still yours to land: please rebase over current main so this PR keeps only that. Note snapshot_project's let _ = swallow (#252) is the highest-value target.

@anyn99

anyn99 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

The premise here — a reported artifact must exist and be non-empty — holds up
well. One case it does not reach yet, found while measuring something else:
exporting a hierarchical schematic to SVG writes one file per sheet.

KiCad 10.0.4, its own bundled pic_programmer demo, through
kicad-cli sch export svg:

input pic_programmer.kicad_sch  ->  pic_programmer.svg
                                    pic_programmer-pic_sockets.svg
input pic_sockets.kicad_sch     ->  pic_sockets.svg

A leaf sheet yields one file; a root with children yields one per sheet.

export_schematic_svg returns output_dir.join("<stem>.svg"), so the
verify_nonempty_file this PR adds covers the root sheet and the child sheets
are neither verified nor reported. On a hierarchical schematic the caller never
learns they were written at all.

This is kicad-cli's own asymmetry rather than anything on this side:

sch export svg  --output OUTPUT_DIR    "Output directory"
sch export pdf  --output OUTPUT_FILE   "Output file"

PDF puts every sheet in one document; SVG cannot, so it takes a directory and
names the files itself.

Worth folding into this PR? Verifying the child files means enumerating them,
and once enumerated, returning them is nearly free. It does change
export_schematic_svg's return type, which #277 is also touching. If you would
rather not, #291 takes it: apply the caller's stem to every file written and
report them all.

@mixelpixx

Copy link
Copy Markdown
Owner

Second reminder, with an offer: this branch still carries both the old #269-content commit (now redundant — #285 landed the consolidated option work) and the artifact-verification commit, and GitHub marks it CONFLICTING. If you'd rather not do the rebase, say so and I'll do it on the fork this week — the #252 verification core is wanted and shouldn't rot here. anyn99's hierarchical-SVG multi-file question from review will also need an answer either way.

@neusse

neusse commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

I tested the proposed salvage path against current main (54b7f84) rather than relying on GitHub's conflict marker. The #252 core is still useful, but I think a clean replacement PR is now safer and easier to review than rewriting this stacked branch.

What I found rebasing it

I transplanted only cda33a3 and dropped 4126705, since #285 already contains the option work. The wanted commit has two straightforward content conflicts:

  • cli.rs: retain the current single_file_pcb_export_args/export-option API and add verification after run_cli.
  • project.rs: retain the current PDF signature and layer names, remove the let _ =, and propagate the PCB export with ?.

After resolving those, the test build still fails because the old artifact-verification test calls the removed pcb_pdf_args() helper. Current main already tests the replacement helper, so that obsolete duplicate test should be removed or rewritten. There is also a Windows-only dead-code warning because result_json is unconditional while its only consumer is #[cfg(unix)].

So this is manageable, but it is not a safe automatic "update branch" operation.

One correctness gap worth fixing before this lands

verify_nonempty_file() proves only that a non-empty file exists after the command. It does not prove that the current invocation produced it.

A reused destination can therefore return a false success:

  1. A non-empty PDF, BOM, position file, Gerber set, or .drl file exists from an earlier run.
  2. The current kicad-cli invocation exits zero but produces nothing.
  3. The verifier finds the stale file and accepts it as the new artifact.

The fresh-temp-directory tests do not exercise this case. Gerber/drill enumeration is especially exposed because it scans an existing directory after the command.

I recommend making freshness structural rather than relying on timestamps:

  • Run single-file exports against a unique sibling temporary path, verify it, then publish it to the requested path.
  • Run Gerber/drill/manufacturing directory exports in a fresh invocation-specific staging directory, verify the complete expected set there, then publish only those verified files.
  • Build the response and upload instructions from that verified manifest; do not tell the caller to upload unrelated pre-existing directory contents.
  • Add regressions where non-empty stale outputs already exist and a fake CLI exits zero without writing anything.

That directly enforces #252's rule that a response is derived from the current result rather than the requested path.

Scope boundary

I would leave hierarchical SVG filename/multi-file behavior to #291. Solving it here changes the response shape and overlaps its explicit design. For this PR/replacement, either omit the schematic-SVG extension or verify only the existing contract without claiming the hierarchy problem is solved.

The critical fake-CLI tests are currently Unix-only. Since the original phantom-path report came from Windows and Windows CI is available, a cross-platform fake-CLI harness or Windows equivalent would make this materially safer.

Suggested way forward

My preference is:

  1. Open a clean replacement PR from current main, focused only on [BUG] snapshot_project returns artifact paths it never verifies #252.
  2. Preserve nordic-style's authorship/credit for the artifact-verification work.
  3. Add stale-output protection and regressions.
  4. Keep [BUG] export_schematic_svg ignores the filename in output while export_schematic_pdf respects it #291 separate.
  5. Close this PR as superseded once the replacement is open.

That gives the verification fix a reviewable history without force-rewriting an inactive contributor branch or carrying obsolete #251/#269 work. If you prefer using maintainer edits on this branch instead, the same changes apply. We can prepare the clean replacement after you choose the direction.

@neusse

neusse commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Superseded by #427, which preserves @nordic-style's original artifact-verification commit and authorship while rebuilding the change on current main. The replacement adds current-invocation staging, stale-destination protection, cross-platform regressions, and real-KiCad evidence. Closing this older branch so there is one review target; its history and credit remain intact.

@neusse neusse closed this Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] snapshot_project returns artifact paths it never verifies

4 participants